Skip to content

Conversation

@arsenm
Copy link
Contributor

@arsenm arsenm commented Oct 19, 2024

These cannot be 0.

@arsenm arsenm marked this pull request as ready for review October 19, 2024 13:25
Copy link
Contributor Author

arsenm commented Oct 19, 2024

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Oct 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 19, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

These cannot be 0.


Full diff: https://github.com/llvm/llvm-project/pull/113038.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+6)
  • (modified) clang/test/CodeGenOpenCL/builtins-amdgcn.cl (+2-1)
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 28f28c70b5ae52..69a7dfc2433ae8 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -18538,6 +18538,12 @@ Value *EmitAMDGPUGridSize(CodeGenFunction &CGF, unsigned Index) {
   auto *GEP = CGF.Builder.CreateGEP(CGF.Int8Ty, DP, Offset);
   auto *LD = CGF.Builder.CreateLoad(
       Address(GEP, CGF.Int32Ty, CharUnits::fromQuantity(4)));
+
+  llvm::MDBuilder MDB(CGF.getLLVMContext());
+
+  // Known non-zero.
+  LD->setMetadata(llvm::LLVMContext::MD_range,
+                  MDB.createRange(APInt(32, 1), APInt::getZero(32)));
   LD->setMetadata(llvm::LLVMContext::MD_invariant_load,
                   llvm::MDNode::get(CGF.getLLVMContext(), std::nullopt));
   return LD;
diff --git a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
index bf5f2971cf118c..be6cee5e9217bf 100644
--- a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
+++ b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -639,7 +639,7 @@ void test_get_workgroup_size(int d, global int *out)
 // CHECK-LABEL: @test_get_grid_size(
 // CHECK: {{.*}}call align 4 dereferenceable(64){{.*}} ptr addrspace(4) @llvm.amdgcn.dispatch.ptr()
 // CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 %.sink
-// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
+// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !range [[$GRID_RANGE:![0-9]+]], !invariant.load
 void test_get_grid_size(int d, global int *out)
 {
 	switch (d) {
@@ -896,5 +896,6 @@ void test_set_fpenv(unsigned long env) {
   __builtin_amdgcn_set_fpenv(env);
 }
 
+// CHECK-DAG: [[$GRID_RANGE]] = !{i32 1, i32 0}
 // CHECK-DAG: [[$WS_RANGE]] = !{i16 1, i16 1025}
 // CHECK-DAG: attributes #[[$NOUNWIND_READONLY]] = { convergent mustprogress nocallback nofree nounwind willreturn memory(none) }

__builtin_amdgcn_set_fpenv(env);
}

// CHECK-DAG: [[$GRID_RANGE]] = !{i32 1, i32 0}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the upper bound is smaller than the lower bound?

Copy link
Contributor Author

@arsenm arsenm Oct 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is how you are supposed to represent the wrapped set where the 0 value isn't allowed but the uint32_max is. See ConstantRange

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I c. range does allow wrap.

@arsenm arsenm force-pushed the users/arsenm/amdgpu-annotate-grid-size-loads branch from 297173f to 5336b21 Compare October 26, 2024 04:14
@arsenm arsenm force-pushed the users/arsenm/clang-amdgpu-mark-grid-size-loads-range-metadata branch from 708215d to 79c3169 Compare October 26, 2024 04:14
@arsenm arsenm force-pushed the users/arsenm/amdgpu-annotate-grid-size-loads branch from 5336b21 to 0bd73d5 Compare October 28, 2024 22:19
@arsenm arsenm force-pushed the users/arsenm/clang-amdgpu-mark-grid-size-loads-range-metadata branch from 79c3169 to 6e74018 Compare October 28, 2024 22:19
@arsenm arsenm force-pushed the users/arsenm/clang-amdgpu-mark-grid-size-loads-range-metadata branch from 6e74018 to 6981d5a Compare November 5, 2024 16:06
@arsenm arsenm requested review from a team as code owners November 5, 2024 16:06
@arsenm arsenm changed the base branch from users/arsenm/amdgpu-annotate-grid-size-loads to main November 5, 2024 16:06
@ldionne
Copy link
Member

ldionne commented Nov 5, 2024

I'm not sure what's special about your usage of Graphite, but I've seen other people use it without triggering these notifications to everyone. This is disruptive: if everyone started doing this, our Github notification system with CODEOWNERS.txt would break down completely because we'd get hundreds of spam notifications a day.

I don't use Graphite, so I don't understand what might be going wrong for certain. But this force-push seems to be the culprit:
Screenshot 2024-11-05 at 11 28 13

If you click on Compare, you'll see it caused the PR to affect like 3000 files, which notified everyone associated to these files. I see that you then updated the base branch to main, which reduced the diff to just your changes. I think you need to do it the other way around: First, change the base branch to main, and then rebase on top of it and force-push. That way, the diff from your branch towards the base branch will never contain more than your own changes.

@ldionne ldionne removed request for a team November 5, 2024 16:34
@nikic nikic removed their request for review November 5, 2024 16:36
@arsenm
Copy link
Contributor Author

arsenm commented Nov 5, 2024

I'm not sure what's special about your usage of Graphite, but I've seen other people use it without triggering these notifications to everyone.

This was specifically a reorder. I've seen this in some reorder cases, but not others

I think you need to do it the other way around: First, change the base branch to main

I am not controlling the process in any way

Copy link
Contributor Author

arsenm commented Nov 5, 2024

Merge activity

  • Nov 5, 3:43 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 5, 3:46 PM EST: Graphite rebased this pull request as part of a merge.
  • Nov 5, 3:47 PM EST: A user merged this pull request with Graphite.

@arsenm arsenm force-pushed the users/arsenm/clang-amdgpu-mark-grid-size-loads-range-metadata branch from 6981d5a to 2e3964f Compare November 5, 2024 20:45
@arsenm arsenm merged commit 0c60573 into main Nov 5, 2024
3 of 5 checks passed
@arsenm arsenm deleted the users/arsenm/clang-amdgpu-mark-grid-size-loads-range-metadata branch November 5, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:AMDGPU clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants